Skip to content

Fix Cookie Clone Issue in Client.Clone() #1036

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

jackcipher
Copy link

@jackcipher jackcipher commented Jun 16, 2025

Problem

There is a critical bug in the cookie cloning logic of Client.Clone() . The current implementation:

cc.cookies = make([]*http.Cookie, l)
for _, cookie := range c.cookies {
    cc.cookies = append(cc.cookies, cloneCookie(cookie))
}

This creates two issues:

  1. The initial make([]*http.Cookie, l) creates a slice with l nil elements
  2. append then adds the actual cookies after these nil elements

Result: A slice with 2l length where:

  • First l elements are nil
  • Last l elements are the actual cookies

image

Panic Example

The following code will trigger a panic:

resty.New().SetCookie(&http.Cookie{
  Name:  "k",
  Value: "v",
 }).Clone(context.Background()).R().Get("about:blank")

This happens because operations like AddCookie may attempt to access the nil cookies, leading to a panic.

Changes Made

  • Fixed cookie cloning in Client.Clone() method
  • Added test cases to verify cookie cloning behavior

Test Coverage

Added new test assertions in TestClientClone to verify:

  • Cookie length equality between parent and clone
  • Cookie name and value equality for each cookie

Impact

This fix ensures that when a client is cloned, all cookies are properly copied to the new instance, maintaining the exact same cookie state as the parent client.

Testing

All existing tests pass, and new test cases have been added to verify the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant